[rom_ctrl, dv] Conditional coverage hole inside the adapter #25482
Conversation
There was a problem hiding this comment.
I agree with the change, but can we expand the reasoning in the commit message a bit? (to explain it for people who weren't working through it with us!)
My reasoning: The only ways that d_valid can set to true require reqfifo_rvalid to be true (checked on line 282).
KinzaQamar
force-pushed
the
tlul_adapter_sram_rtl
branch
from
dee3ec6 to
81a875e
Compare
|
CHANGE AUTHORIZED: hw/ip/tlul/rtl/tlul_adapter_sram.sv |
There was a problem hiding this comment.
I agree that the logical implication d_valid requires reqfifo_rvalid holds in the current implementation, based on lines 282 to 292 of tlul_adapter_sram.sv. Thus reqfifo_rvalid in d_valid & reqfifo_rvalid is redundant and can be removed.
Maybe we break this implication in the future, though, so I think it would be valuable to add an assertion for this as we prune the logical AND above. If that makes sense to you, could you please expand your commit?
KinzaQamar
force-pushed
the
tlul_adapter_sram_rtl
branch
3 times, most recently
from
58ef1aa to
d67c796
Compare
KinzaQamar
force-pushed
the
tlul_adapter_sram_rtl
branch
from
d67c796 to
e6202e8
Compare
KinzaQamar
force-pushed
the
tlul_adapter_sram_rtl
branch
from
e6202e8 to
ae9a192
Compare
|
CHANGE AUTHORIZED: hw/ip/tlul/rtl/tlul_adapter_sram.sv No functional change, as ensured by the newly added assertion |
** There is a removal of an item (reqfifo_rvalid) from the conditional statement to deduct coverage hole 1011 for rom_ctrl .We can't see dvalid & !reqfifo_rvalid. Reason being if d_valid is true then reqfifo_rvalid must be true. Similarly, if reqfifo_rvalid is false, then we can't see d_valid high. ** An assertion is added for the d_valid -> reqfifo_rvalid to make sure that it can never be true for the adapter. Signed-off-by: Kinza Qamar <[email protected]>
KinzaQamar
force-pushed
the
tlul_adapter_sram_rtl
branch
from
ae9a192 to
34bf831
Compare
KinzaQamar
changed the title
There is a removal of an item (reqfifo_rvalid) to remove the conditional coverage hole 1011. If reqfifo_rvalid is 0, then we won't see d_valid.